Skip to content

repl: fix history truncation of long lines#15258

Merged
Ericson2314 merged 1 commit intoNixOS:masterfrom
obsidiansystems:fix-repl-history
Mar 10, 2026
Merged

repl: fix history truncation of long lines#15258
Ericson2314 merged 1 commit intoNixOS:masterfrom
obsidiansystems:fix-repl-history

Conversation

@amaanq
Copy link
Copy Markdown
Member

@amaanq amaanq commented Feb 16, 2026

Motivation

Editline's read_history uses fgets(buf, 256, fp) internally, splitting
any line longer than 255 characters into multiple history entries. This
commit bypasses it with std::getline and add_history calls, which
handle arbitrary lengths. GNU readline's read_history has no such limit
and is left as-is, and write_history already writes full lines via
fprintf so it needs no change either.

Context

Add 👍 to pull requests you find important.

The Nix maintainer team uses a GitHub project board to schedule and track reviews.

@amaanq amaanq requested a review from edolstra as a code owner February 16, 2026 22:38
@amaanq amaanq force-pushed the fix-repl-history branch 5 times, most recently from 0d5de78 to 1c092de Compare February 17, 2026 02:07
xokdvium
xokdvium reviewed Feb 23, 2026
View reviewed changes
xokdvium
xokdvium reviewed Feb 23, 2026
View reviewed changes
@amaanq amaanq force-pushed the fix-repl-history branch 2 times, most recently from 30f1e1f to 2e03021 Compare February 23, 2026 16:01
@amaanq amaanq requested a review from xokdvium February 23, 2026 17:50
@amaanq amaanq force-pushed the fix-repl-history branch 2 times, most recently from 9999814 to 153ff2f Compare February 24, 2026 00:12
Ericson2314
Ericson2314 reviewed Feb 26, 2026
View reviewed changes
@github-actions github-actions bot added the new-cli Relating to the "nix" command label Feb 26, 2026
@amaanq amaanq force-pushed the fix-repl-history branch 2 times, most recently from 4e74cdf to bca37f6 Compare February 26, 2026 00:54
@Ericson2314 Ericson2314 mentioned this pull request Feb 26, 2026
Merged
xokdvium
xokdvium reviewed Mar 7, 2026
View reviewed changes
@amaanq amaanq force-pushed the fix-repl-history branch 4 times, most recently from 3f8b0da to 79e4f2c Compare March 9, 2026 22:24
@amaanq
repl: fix history truncation of long lines
12bdab6 
Editline's `read_history` uses `fgets(buf, 256, fp)` internally, splitting
any line longer than 255 characters into multiple history entries. This
commit bypasses it with `FdSource::readLine` and per-line `add_history`
calls, which handle arbitrary lengths. GNU readline's `read_history` has no
such limit and is left as-is, and `write_history` already writes full lines
via `fprintf` so it needs no change either.
@amaanq amaanq force-pushed the fix-repl-history branch from 79e4f2c to 12bdab6 Compare March 9, 2026 22:30
@Ericson2314 Ericson2314 enabled auto-merge March 9, 2026 22:30
@Ericson2314 Ericson2314 added this pull request to the merge queue Mar 9, 2026
Merged via the queue into NixOS:master with commit a2c3a96 Mar 10, 2026
15 checks passed
@Ericson2314 Ericson2314 deleted the fix-repl-history branch March 10, 2026 00:12
@andir
Copy link
Copy Markdown
Member

andir commented Mar 10, 2026

I don't thinkt the test is working. When I run the repl through unbuffer it does indeed not crash any more. When I just run the repl from the current master branch it will crash while calling hist_add during the repl init.

gdb) bt
#0  0x00007fb4da9a0706 in hist_add () from /nix/store/n0jys3rah5s1162iy4yps7zli7fg8qgz-editline-1.17.1-unstable-2025-05-24/lib/libeditline.so.1
#1  0x00007fb4dbef70de in nix::ReadlineLikeInteracter::init(nix::detail::ReplCompleterMixin*) ()
   from /nix/store/5fi3fhl1c4gs38ca0v91ffb1yq9raz4w-nix-cmd-2.35.0pre/lib/libnixcmd.so.2.35.0
#2  0x00007fb4dbef732a in nix::NixRepl::mainLoop() () from /nix/store/5fi3fhl1c4gs38ca0v91ffb1yq9raz4w-nix-cmd-2.35.0pre/lib/libnixcmd.so.2.35.0
#3  0x00005603f1797eb0 in nix::CmdRepl::run(nix::ref<nix::Store>, std::vector<std::__cxx11::basic_string<char, std::char_traits<char>, std::allocator<char> >, std::allocator<std::__cxx11::basic_string<char, std::char_traits<char>, std::allocator<char> > > >&&) ()
#4  0x00007fb4dbee9041 in nix::RawInstallablesCommand::run(nix::ref<nix::Store>) ()
   from /nix/store/5fi3fhl1c4gs38ca0v91ffb1yq9raz4w-nix-cmd-2.35.0pre/lib/libnixcmd.so.2.35.0
#5  0x00007fb4dbebe7af in nix::StoreCommand::run(nix::ref<nix::StoreConfig>) ()
   from /nix/store/5fi3fhl1c4gs38ca0v91ffb1yq9raz4w-nix-cmd-2.35.0pre/lib/libnixcmd.so.2.35.0
#6  0x00007fb4dbebe908 in virtual thunk to nix::StoreConfigCommand::run() ()
   from /nix/store/5fi3fhl1c4gs38ca0v91ffb1yq9raz4w-nix-cmd-2.35.0pre/lib/libnixcmd.so.2.35.0
#7  0x00005603f176490d in nix::mainWrapped(int, char**) ()
#8  0x00007fb4dca63925 in nix::handleExceptions(std::__cxx11::basic_string<char, std::char_traits<char>, std::allocator<char> > const&, nix::fun<void ()>) () from /nix/store/yhxk8iyb62hkjss61l5a29x9xsbriyxx-nix-util-2.35.0pre/lib/libnixutil.so.2.35.0
#9  0x00005603f16c0513 in main ()
(gdb) disas
Dump of assembler code for function hist_add:
   0x00007fb4da9a06a0 <+0>:	push   %rbp
   0x00007fb4da9a06a1 <+1>:	mov    %rsp,%rbp
   0x00007fb4da9a06a4 <+4>:	push   %r15
   0x00007fb4da9a06a6 <+6>:	push   %r14
   0x00007fb4da9a06a8 <+8>:	push   %r13
   0x00007fb4da9a06aa <+10>:	push   %r12
   0x00007fb4da9a06ac <+12>:	mov    %rdi,%r12
   0x00007fb4da9a06af <+15>:	push   %rbx
   0x00007fb4da9a06b0 <+16>:	sub    $0x8,%rsp
   0x00007fb4da9a06b4 <+20>:	mov    0x92a6(%rip),%ebx        # 0x7fb4da9a9960 <H>
   0x00007fb4da9a06ba <+26>:	test   %ebx,%ebx
   0x00007fb4da9a06bc <+28>:	je     0x7fb4da9a06d7 <hist_add+55>
   0x00007fb4da9a06be <+30>:	mov    0x92a3(%rip),%rdx        # 0x7fb4da9a9968 <H+8>
   0x00007fb4da9a06c5 <+37>:	lea    -0x1(%rbx),%eax
   0x00007fb4da9a06c8 <+40>:	cltq
   0x00007fb4da9a06ca <+42>:	mov    (%rdx,%rax,8),%rsi
   0x00007fb4da9a06ce <+46>:	call   0x7fb4da9a0200 <strcmp@plt>
   0x00007fb4da9a06d3 <+51>:	test   %eax,%eax
   0x00007fb4da9a06d5 <+53>:	je     0x7fb4da9a0711 <hist_add+113>
   0x00007fb4da9a06d7 <+55>:	mov    %r12,%rdi
   0x00007fb4da9a06da <+58>:	call   0x7fb4da9a0360 <strdup@plt>
   0x00007fb4da9a06df <+63>:	mov    %rax,%r12
   0x00007fb4da9a06e2 <+66>:	test   %rax,%rax
   0x00007fb4da9a06e5 <+69>:	je     0x7fb4da9a0711 <hist_add+113>
   0x00007fb4da9a06e7 <+71>:	mov    0x88aa(%rip),%r14        # 0x7fb4da9a8f98
   0x00007fb4da9a06ee <+78>:	mov    0x9273(%rip),%r13        # 0x7fb4da9a9968 <H+8>
   0x00007fb4da9a06f5 <+85>:	cmp    (%r14),%ebx
   0x00007fb4da9a06f8 <+88>:	jg     0x7fb4da9a0730 <hist_add+144>
   0x00007fb4da9a06fa <+90>:	lea    0x1(%rbx),%eax
   0x00007fb4da9a06fd <+93>:	mov    %eax,0x925d(%rip)        # 0x7fb4da9a9960 <H>
   0x00007fb4da9a0703 <+99>:	movslq %ebx,%rax
=> 0x00007fb4da9a0706 <+102>:	mov    %r12,0x0(%r13,%rax,8)
   0x00007fb4da9a070b <+107>:	mov    %ebx,0x9253(%rip)        # 0x7fb4da9a9964 <H+4>
   0x00007fb4da9a0711 <+113>:	add    $0x8,%rsp
   0x00007fb4da9a0715 <+117>:	pop    %rbx
   0x00007fb4da9a0716 <+118>:	pop    %r12
   0x00007fb4da9a0718 <+120>:	pop    %r13
--Type <RET> for more, q to quit, c to continue without paging--q
❌️ Quit
(gdb)

I'm not entirely sure whats going on there.. but perhaps we are missing some other initialization code.

The || true in the repl test command is also kind of strange. Why is that needed? Wouldn't it silence things like a non-zero exit call if that were still the case?

@xokdvium
Copy link
Copy Markdown
Contributor

xokdvium commented Mar 10, 2026

Meh, somebody forgor to call rl_initialize. Will put up a fix now.

@Ericson2314, @amaanq please be more careful and at least run the code. I'm going to fix this mess, but I don't enjoy having to constantly clean up regressions.

@xokdvium
Copy link
Copy Markdown
Contributor

xokdvium commented Mar 10, 2026

What's even the purpose of unbuffer here? The comment is completely wrong, just piping in the input is enough to catch this bug. This is a flaming pile of garbage.

@Ericson2314
Copy link
Copy Markdown
Member

Ericson2314 commented Mar 10, 2026

Sorry @xokdvium, I thought (though didn't confirm with anyone) that you and @amaanq and talked about this more, and had already gone over things and all that was left was the maybe function stuff.

brittonr pushed a commit to brittonr/nix that referenced this pull request Apr 1, 2026
@Ericson2314
Merge pull request NixOS#15258 from obsidiansystems/fix-repl-history
6e702c4 
repl: fix history truncation of long lines
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@xokdvium xokdvium xokdvium left review comments

@Ericson2314 Ericson2314 Ericson2314 approved these changes

@edolstra edolstra Awaiting requested review from edolstra edolstra is a code owner

Assignees

No one assigned

Labels

new-cli Relating to the "nix" command

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

nix repl splits long lines in repl-history

4 participants

@amaanq @andir @xokdvium @Ericson2314